implement CCIPAPIClient.getExecutionInput#155
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
0176a1f to
2c87434
Compare
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
a54e4fd to
3347926
Compare
PabloMansanet
left a comment
PabloMansanet
left a comment
There was a problem hiding this comment.
Looking good, just two minor comments. I'll handle the fork test early Monday.
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
7d3e5c2 to
76310cf
Compare
Coverage Report |
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
12abb78 to
9dd1853
Compare
| } | ||
|
|
||
| const messagesInBatch = raw.messageBatch.map(decodeMessage) | ||
| const message = messagesInBatch.find((message) => message.messageId === messageId)! |
There was a problem hiding this comment.
What's the behaviour if the message is not found? (in runtime this will become undefined)
There was a problem hiding this comment.
It should NEVER not be found, if API is doing what it should; if it can't, then it's expected the API itself will return an HTTP error. It's basically an invariant of the function of the endpoint, that it should include the message we're looking in the batch for its messageId
|
|
||
| const proof = calculateManualExecProof(messagesInBatch, lane, messageId, raw.merkleRoot, this) | ||
|
|
||
| const rawMessage = raw.messageBatch.find((message) => message.messageId === messageId)! |
There was a problem hiding this comment.
What's the behaviour if the rawMessage is not found? (in runtime this will become undefined)
There was a problem hiding this comment.
Same. The message MUST be there, if the API does what it's supposed to; any error should be thrown at http response level
| offRamp, | ||
| version: CCIPVersion.V2_0, | ||
| encodedMessage: raw.encodedMessage, | ||
| verifications: (raw.ccvData ?? []).map((ccvData, i) => ({ |
There was a problem hiding this comment.
I'd suggest some defensive coding in case there is a bug in the API, which will raise an error earlier instead of passing it throgh the layers (which will be harder to investigate) . imho, we should validate that ccvData.length === verifierAddresses.length
There was a problem hiding this comment.
Hmm, I'd say it's not sdk's responsibility to verify behavior of the API, but just to assume and use it. It's more or less the same with RPC: we must assume a correctly working data source layer, otherwise nothing will ever be able to be achieved here. At most, we validate for things that CAN happen in normal operation
| destChainSelector: bigint | ||
| version: string | ||
| } | ||
| | object |
There was a problem hiding this comment.
It's linter lang for {}, but it doesn't like the empty type; the difference between this union and just making all those properties optional, is that it tells TS that, if any is present, all of them are, while optionals I'd need to check one by one
| sourceChainSelector: raw.sourceChainSelector, | ||
| destChainSelector: raw.destChainSelector, | ||
| onRamp: raw.onramp, | ||
| version: raw.version as CCIPVersion, |
There was a problem hiding this comment.
Why don't you validate the value like we do for other part in the code (validateChainFamily / validateMessageStatus) just for the sake of defensive coding
There was a problem hiding this comment.
We don't do much validation; in this case, I'd argue we WANT this endpoint to support "unexpected" versions, as this may allow for things to continue working optimistically in case the SDK is outdated with CCIP's version
There was a problem hiding this comment.
But I agree, we do some validation elsewhere, also coming from the API, so maybe we should do it here. I'll implement
There was a problem hiding this comment.
Ok, I thought more on this, and I think it's fine to assume the API will just return something supported; we can check later if the need to validate this kind of thing becomes a real issue; for now, we control this e2e, so better keep the code simple and minimize validations from trusted sources
ccip-sdk/src/api/index.ts
Outdated
| CCIPTimeoutError, | ||
| CCIPUnexpectedPaginationError, | ||
| } from '../errors/index.ts' | ||
| import { decodeMessageV1 } from '../evm/messages.ts' |
There was a problem hiding this comment.
can we move decodeMessageV1 out of evm/messages.ts to another file that doesn't import ./const.ts? Otherwise it will pull ABI files in the bundle even for users who are not using EVM (even users who only use CCIPAPIClient)
There was a problem hiding this comment.
Sure, good catch!
And use it in EVMChain.[generateUnsigned]Execute({ messageId }) method
useful if you're executing with `dest.execute({ messageId })` only
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
9dd1853 to
0d64bf3
Compare
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
2a44446 to
397d10d
Compare
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
397d10d to
bbeda0f
Compare
PabloMansanet
left a comment
PabloMansanet
left a comment
There was a problem hiding this comment.
LGTM, just two clarity/readability nits. Maybe it would be good to find other messages to add to the fork test (i.e. token transfers, different lanes) for full confidence, but that can be a follow-up 👍
ccip-sdk/src/api/index.ts
Outdated
| // v2 messages | ||
| const { | ||
| sourceChainSelector, | ||
| destChainSelector, | ||
| onRampAddress: onRamp, | ||
| } = decodeMessageV1(raw.encodedMessage) |
There was a problem hiding this comment.
This is correct but reads strangely, "v2 messages" and "decodeMessageV1". Maybe let's reword the comment to "2.0 lane messages".
There was a problem hiding this comment.
Ok, I'll add a comment to clarify this a little
| let gasLimit = await this.provider.estimateGas(manualExecTx) | ||
| if ( | ||
| 'gasLimit' in input.message && | ||
| input.message.gasLimit && | ||
| gasLimit < input.message.gasLimit + 100000n | ||
| ) | ||
| gasLimit = BigInt(input.message.gasLimit) + 200000n | ||
| else if ('gasLimit' in input.message && !input.message.gasLimit && gasLimit < 240000n) | ||
| gasLimit = 240000n | ||
| manualExecTx.gasLimit = gasLimit |
There was a problem hiding this comment.
There's some deep magic here, would be good to leave a comment as to where these numbers are calculated and where they're coming from.
There was a problem hiding this comment.
This is just "what works" for v1 execs, as I had to test empirically and get those to work during the last tx spike, where this code exec'ed 95k messages. I can add some comments to explain a little bit why they're needed here
andrevmatos
force-pushed
the
api/get_execution_input
branch
from
bbeda0f to
e994888
Compare
4 participants
/execution-inputsAPI endpoint for both v1 and v2 messagesdest.execute({ messageId })manual-execwhen given amessageIdsource: